Skip to content

Use span directly instead of reader / writer for methods & properties #936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Aug 31, 2020
Merged

Use span directly instead of reader / writer for methods & properties #936

merged 9 commits into from
Aug 31, 2020

Conversation

bollhals
Copy link
Contributor

Proposed Changes

  1. Introducing a benchmark project to store actual benchmark code.
  2. Replace the ContentHeaderProperty(Reader/Writer) & MethodArgument(Reader/Writer) by directly using the span.

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

Results:
WireFormatting_Read_BasicAck

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
ReadArgumentsFrom_MethodArgumentReader 6.949 ns 14.0979 ns 0.7728 ns 1.55 0.0068 - - 32 B
ReadFromSpan 4.486 ns 0.2777 ns 0.0152 ns 1.00 0.0068 - - 32 B

WireFormatting_Read_BasicDeliver

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
ReadArgumentsFrom_MethodArgumentReader 23.91 ns 0.634 ns 0.035 ns 1.80 0.0119 - - 56 B
ReadFromSpan 13.28 ns 3.272 ns 0.179 ns 1.00 0.0119 - - 56 B

WireFormatting_Read_ChannelClose

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
ReadArgumentsFrom_MethodArgumentReader 14.261 ns 0.6648 ns 0.0364 ns 1.70 0.0068 - - 32 B
ReadFromSpan 8.343 ns 0.3412 ns 0.0187 ns 1.00 0.0068 - - 32 B

WireFormatting_Write_BasicAck

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
WriteArgumentsTo_MethodArgumentWriter 5.110 ns 0.2012 ns 0.0110 ns 2.47 - - - -
WriteArgumentsTo 2.066 ns 0.0219 ns 0.0012 ns 1.00 - - - -

WireFormatting_Write_BasicDeliver

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
WriteArgumentsTo_MethodArgumentWriter 45.72 ns 0.908 ns 0.050 ns 1.15 - - - -
WriteArgumentsTo 39.71 ns 1.600 ns 0.088 ns 1.00 - - - -

WireFormatting_Write_ChannelClose

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
WriteArgumentsTo_MethodArgumentWriter 19.72 ns 1.575 ns 0.086 ns 1.29 - - - -
WriteArgumentsTo 15.24 ns 0.138 ns 0.008 ns 1.00 - - - -

WireFormatting_Read_BasicProperties

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
ReadFrom 96.78 ns 3.470 ns 0.190 ns 1.35 0.0408 - - 192 B
ReadFromSpan 71.50 ns 0.860 ns 0.047 ns 1.00 0.0408 - - 192 B

WireFormatting_Write_BasicProperties

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
WriteProperties 63.90 ns 3.239 ns 0.178 ns 1.74 - - - -
WritePropertiesToSpan 36.68 ns 0.331 ns 0.018 ns 1.00 - - - -

From the test app:
Before:
image
image

After:
image
image

@lukebakken lukebakken added this to the 7.0.0 milestone Aug 28, 2020
@michaelklishin
Copy link
Contributor

@bollhals thank you, this looks reasonable. Can you please rebase this on top of master?

@michaelklishin michaelklishin merged commit b3e7a14 into rabbitmq:master Aug 31, 2020
@bollhals bollhals deleted the useSpanDirectly branch August 31, 2020 23:25
@michaelklishin
Copy link
Contributor

@bollhals if this could go into 7.x, would it be possible to submit a version of this based on that branch? If anything, these changes would make it easier to backport other changes to 7.x: the delta with master keeps growing.

@bollhals
Copy link
Contributor Author

@bollhals if this could go into 7.x, would it be possible to submit a version of this based on that branch? If anything, these changes would make it easier to backport other changes to 7.x: the delta with master keeps growing.

Sure that's possible. I'm wondering is there any change on master that is really 8.0 only? Or couldn't we merge master to 7.0?

@lukebakken lukebakken modified the milestones: 8.0.0, 7.0.0 Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelklishin michaelklishin michaelklishin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bollhals @michaelklishin @lukebakken